-
Notifications
You must be signed in to change notification settings - Fork 602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A patch to intergrate Flickity with React #381
base: master
Are you sure you want to change the base?
Conversation
Thank you for this contribution. As a non-React user, this is foreign for me. I'm not sure if I want Flickity to go down this path. Flickity was designed with a browser environment in mind, at nearly every level. Removing DOM from Flickity and its dependencies would be a complicated endeavor. Even then, I'm not sure how successful Flickity would work in a server-side environment. It simply was not built for this use case. I appreciate your effort here, but I do not feel comfortable merging in this code. I'll keep this PR open for a bit so others can provide feedback. |
In my opinion I find it a bit useless. I think that this feature would be achived only rewriting (a big?) part of the flickity's code. Btw i liked the unusual purpose. Just my opinion, do not kill me please 😂. |
@desandro afaik, Flickity can't work in a server environment at least because it uses client side features moving slides back and forth. But this is not the case ;) By the way, initially, I used @PaoloFalomo I'm not the first one. There is at least one gist in google for this =) Both of those assumes cell configuration is static. While if you have to filter out some cells and reload them, Flickity behaves not that well as you might want it ... So, as a resume: this is not intended to be in a server side render. Nor this is a replacement for DOM manipulation in Flickity. This is a small feature to delegate some work to an outside framework. |
js/flickity.js
Outdated
this.viewport = document.createElement('div'); | ||
this.viewport.className = 'flickity-viewport'; | ||
if (this.options.noDomMod) { | ||
this.viewport = document.querySelector('.flickity-viewport'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oxpa
is this working with multiple flickity sliders too?
querySelector()
references only to the first element found with that selection but if there're more '.flickity-viewport'
(eg. with more than 1 slider) it will take just the first found discarding others.
I'm just mind-debugging and i think that could ✅ work if you run the querySelector()
in an inner element instead of looking throw all the document
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... You are right: I didn't keep in mind, there might be several instances. I'll have this changed in awhile
I have updated the patch and example to work with multiple Flickity instances. (Still they share react state so will have the same amount of cells, sorry =) And I'm still not sure about the 'reloadCells' hunk. |
Oh wow @oxpa thanks so much for this! I've been fighting all day with how to get Flickity to work inside of a React app. This did the trick! @desandro Please consider accepting this or something quite similar. React handles all DOM manipulation itself via a virtual DOM implementation. When a plugin/library/etc. modifies the DOM outside of that, React doesn't really know what to do and many times will fail. By providing the |
Conflicts: js/flickity.js
This reverts commit 170ff65. removing these results in messed up cells on flickity enabling-disabling
Just echoing the above, this patch has been helpful for me integrating Flickity in a React app with SSR |
This is similar to #488 – I handle the addition/removing cells in React though, if the props that I'm rendering my children with, I call |
+1 This patch is also required to get Flickity to work in an advanced VueJS setup. Same problems, same solution. |
To offer a perhaps slightly different explanation of the problem that this PR is trying to solve: Libraries like React and Vue which rely on virtual DOM implementations (an object tree structure that mirrors the actual DOM), get confused when you modify DOM structure manually because the actual DOM no longer mirrors the structure of the virtual one. Flickity transforms your markup on instantiation by wrapping your cells in More and more developers are moving towards libraries that take advantage of virtual DOM, so I think it is definitely a valid use case for Flickity. |
@wolthers Thanks for providing more insight on this PR. I'm still evaluating the best implementation. |
@desandro Could you share with us your plans in terms of support for integration with react? I really consider buying commercial license but React support is a must-have in my case. Happy new year! 🎉 🎈 🍾 |
This PR is also a must when trying to integrate flickity with prosemirror's nodeView. It allows one to edit this carousel in a wysiwyg editor (granted, you need a certain amount of work to achieve this). |
👍 +1 For this PR. The changes are pretty innocuous. The offending elements that are added by Flickity, namely; flickity-viewport and flickity-slider, are not consumed in a way that a wrong declaration can break Flickity. Unless there are some future plans which render this assumption false then I propose an option of sort is made into the next release. I would suggest a more meaningful name, noDomMod works, but something more explicit like Another suggestion, specifically for this patch. Is that I don't agree with the suppression of both Nitpicking now 😄 I would consider using |
This is probably relevant here too: #488 (comment) |
Self-promotion here: please check out react-flickity-component for using with react. |
# Conflicts: # js/flickity.js
Add PageDots control to noDomMod
Also for working in CMS, would this be a very nice and essential feature. When do you plan to merge this? |
Hello.
First of all, I have to admit, I'm not a js programmer. I have no knowledge of npm or whatever. So this is more a feature request than a pull request =)
Still, this pull request consists of two changes.
The first one introduced 'noDomMod' option for Flickity. This option prevents DOM modifications inside Flickity.
Why would anyone need this? When I used Flickity with React I realized, Flickity changes DOM and React fails (because React checks that DOM matches React's internal shadow DOM). The change Flickity made is trivial: only two div elements are added and cells are moved into them.
So the noDomMod patch is about basic react compatibility. In my case, I used react to filter out only matching tabs. But here is a simplier example, which just adds and removes cells:
https://codepen.io/oxpa/pen/VQmNdN (sorry for a ton of JS, but this is how I'm using react...)
But there are several problems with this rendering. As you might notice, in the demo I'm using
.activate()
anddeactivate()
while I would prefer.reloadCells()
. First of all, reload doesn't update dots. And I didn't work around this issue. Secondly, reload mess with cells if I don't reset before and after wrap arrays. And this is the second commit: normalize wrapping behaviour on external DOM management.So, basically, could anyone please review this and propose a
reloadCells
better patch?(I read the contributing guideline, and lets agree, this is a mit licensed pull request)